-
Notifications
You must be signed in to change notification settings - Fork 293
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[FIRRTL] Don't force non-local trackers in Dedup. #7709
base: main
Are you sure you want to change the base?
Conversation
The previous behavior for local trackers would be to clone the tracker into duplicate copies for each unique instance. This will fail in LowerClasses, because one path op that used to refer to a single entity as a local reference would now refer to every hierarchical instance of that entity. One path can't refer to multiple things, so if the user specified a local reference, allow that to pass through without expanding it into every hierarchical instance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, but do we need to handle ambiguous paths in LowerClass because of this ?
Nope, I don't think I don't think a local target is ambiguous. If a user made a path like The difference in this change is if Foo happened to dedupe with something else, this |
How do we end up with duplicated local trackers, anyways? Maybe the inliner could do it? |
Likely inliner, potentially others. I need to chase this down and resolve it. I added dedupe of local trackers here in Dedup similar to how we do it for the local dont touch annotations, but in some testing downstream I'm seeing other sources of duplicated local trackers that I need to chase down. |
I think the last commit is still needed here. But the problem I was seeing downstream was actually an interaction with PrefixModules, which can also lead to duplicate annotations in cases like the examples here. That is more fundamental; I don't think there is anything we can do there with the current representation of trackers as annotations, so we'll need to hold this until module prefixes are applied by Chisel and not annotations. I do think we'll want this change at some point. |
And, now that I think about this, I may have been mistaken; the last commit here was probably not needed, and was me confusing the interaction with PrefixModules with the logic in Dedup. |
The previous behavior for local trackers would be to clone the tracker into duplicate copies for each unique instance. This will fail in LowerClasses, because one path op that used to refer to a single entity as a local reference would now refer to every hierarchical instance of that entity. One path can't refer to multiple things, so if the user specified a local reference, allow that to pass through without expanding it into every hierarchical instance.